Skip to content

Refresh and automate updating licenses list - #489

Merged
Secrus merged 2 commits into
python-poetry:mainfrom
Secrus:refresh-and-automate-licenses
Aug 10, 2023
Merged

Refresh and automate updating licenses list#489
Secrus merged 2 commits into
python-poetry:mainfrom
Secrus:refresh-and-automate-licenses

Conversation

@Secrus

@Secrus Secrus commented Oct 3, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Resolves: python-poetry/poetry#6689

  • Added tests for changed code. (no significant changes)
  • Updated documentation for changed code. (internal changes)

colindean
colindean previously approved these changes Oct 4, 2022
View reviewed changes

@colindean colindean left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just about picked this up myself as a Hacktoberfest project

@colindean colindean mentioned this pull request Oct 4, 2022
Closed
3 of 4 tasks
@neersighted

neersighted commented Oct 10, 2022

Copy link
Copy Markdown
Member

@Secrus would you mind dropping the update, and adding a workflow_dispatch event so we can manually trigger your workflow? I'd rather do that so we can test the workflow now and fix any issues.

neersighted
neersighted previously requested changes Oct 10, 2022
View reviewed changes
Comment thread src/poetry/core/spdx/license.py Resolved
Comment thread src/poetry/core/spdx/license.py Resolved
Comment thread src/poetry/core/spdx/license.py Resolved
@sonarqubecloud

sonarqubecloud Bot commented Oct 11, 2022

Copy link
Copy Markdown

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@Secrus

Secrus commented Oct 11, 2022

Copy link
Copy Markdown
Member Author

@neersighted I have added the event to the workflow and reverted changes (don't worry about the commit history, I will clean it up before merging)

mkniewallner
mkniewallner reviewed Oct 11, 2022
View reviewed changes

@mkniewallner mkniewallner left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if there's a valid reason for having the updater in poetry-core itself. Wouldn't it be more suitable to have it in a dedicated scripts directory outside the package?

Comment thread .github/workflows/update_licenses.yml Resolved
@mkniewallner mkniewallner mentioned this pull request Oct 29, 2022
Closed
2 tasks done
@Secrus
Secrus force-pushed the refresh-and-automate-licenses branch from 7b2a532 to dbaf122 Compare November 28, 2022 03:24
@sonarqubecloud

sonarqubecloud Bot commented Nov 28, 2022

Copy link
Copy Markdown

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@Secrus

Secrus commented Nov 28, 2022

Copy link
Copy Markdown
Member Author

@neersighted @mkniewallner I reduced this PR to just a workflow, I will be refactoring licensing in another PR.

@Secrus
Secrus requested review from colindean, mkniewallner and neersighted and removed request for colindean November 28, 2022 03:26
colindean
colindean previously approved these changes Nov 30, 2022
View reviewed changes
Comment thread .github/workflows/update_licenses.yml Resolved
eamanu
eamanu previously approved these changes May 13, 2023
View reviewed changes

@eamanu eamanu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, this PR LGTM. It would great have the license.py updated. Any possibility to merge it?

@eamanu

eamanu commented May 22, 2023

Copy link
Copy Markdown
Contributor

Hi, @Secrus Are you still interest to continue this PR? :-)

@Secrus

Secrus commented May 22, 2023

Copy link
Copy Markdown
Member Author

@eamanu Yeah, I will try to merge it this week

@Secrus
Secrus dismissed stale reviews from eamanu and colindean via 4ad85d2 May 23, 2023 22:21
@Secrus
Secrus force-pushed the refresh-and-automate-licenses branch from dbaf122 to 4ad85d2 Compare May 23, 2023 22:21
@Secrus
Secrus requested a review from a team May 23, 2023 22:23
@Secrus
Secrus force-pushed the refresh-and-automate-licenses branch from 4ad85d2 to f6a3c4d Compare June 2, 2023 23:12
@sonarqubecloud

sonarqubecloud Bot commented Jun 2, 2023

Copy link
Copy Markdown

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

radoering
radoering reviewed Jun 3, 2023
View reviewed changes

@radoering radoering left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since no member with more experience in github actions seems to be available, I'll do my best and probably ask some stupid questions. 😉

Comment thread .github/workflows/update_licenses.yml Resolved
@Secrus
Secrus force-pushed the refresh-and-automate-licenses branch 2 times, most recently from 839d37d to be1424d Compare July 15, 2023 12:46
@Secrus
Secrus force-pushed the refresh-and-automate-licenses branch from be1424d to 96c7f6f Compare July 15, 2023 12:49
@Secrus
Secrus requested a review from radoering July 20, 2023 15:43
@Secrus Secrus mentioned this pull request Jul 28, 2023
Closed
radoering
radoering reviewed Jul 30, 2023
View reviewed changes
Comment thread .github/workflows/update_licenses.yml Resolved
Comment thread .github/workflows/update_licenses.yml Resolved
@Secrus
Secrus force-pushed the refresh-and-automate-licenses branch from 96c7f6f to 6b006ee Compare July 30, 2023 23:13
@Secrus
Secrus force-pushed the refresh-and-automate-licenses branch from 6b006ee to 38394b2 Compare July 30, 2023 23:14
@radoering

radoering commented Jul 31, 2023

Copy link
Copy Markdown
Member

LGTM to me now. Can we test it before merging as proposed in #489 (comment)?

radoering
radoering approved these changes Aug 10, 2023
View reviewed changes

@radoering radoering left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably too much effort to test before merging so let's try.

@sonarqubecloud

sonarqubecloud Bot commented Aug 10, 2023

Copy link
Copy Markdown

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@Secrus
Secrus removed the request for review from neersighted August 10, 2023 19:35
@Secrus
Secrus dismissed neersighted’s stale review August 10, 2023 19:36

no longer valid

@Secrus
Secrus merged commit e02ee6b into python-poetry:main Aug 10, 2023
@dimbleby

dimbleby commented Aug 10, 2023

Copy link
Copy Markdown
Contributor

are you in a suitable virtual environment when you call the script? won't it just fail with 'No module named poetry'?

@Secrus

Secrus commented Aug 10, 2023

Copy link
Copy Markdown
Member Author

are you in a suitable virtual environment when you call the script? won't it just fail with 'No module named poetry'?

of course it will, (and actually did), but we accepted that risk. See #622 for fixes

@dimbleby

dimbleby commented Aug 10, 2023

Copy link
Copy Markdown
Contributor

are you in a suitable virtual environment when you call the script? won't it just fail with 'No module named poetry'?

ah never mind, #622, I am too slow

@Secrus
Secrus deleted the refresh-and-automate-licenses branch August 11, 2023 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@neersighted neersighted neersighted left review comments
@radoering radoering radoering approved these changes
@mkniewallner mkniewallner Awaiting requested review from mkniewallner
+3 more reviewers
@edgarrmondragon edgarrmondragon edgarrmondragon left review comments
@colindean colindean colindean left review comments
@eamanu eamanu eamanu left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

The Unlicense not recognized as a license

8 participants

@Secrus @neersighted @eamanu @radoering @dimbleby @colindean @mkniewallner @edgarrmondragon